CP-53711: Copy SSH settings from pool coordinator in pool join by gangj · Pull Request #6395 · xapi-project/xen-api

gangj · 2025-04-01T03:00:36Z

No description provided.

BengangY · 2025-04-01T03:27:03Z

ocaml/idl/datamodel_pool.ml

        ; param_name= "username"
        ; param_doc= "The username of the remote pool"
-        ; param_release= numbered_release "25.6.0-next"
+        ; param_release= numbered_release "25.7.0"


Should we update these release versions when the feature branch is ready to be merged into the master branch?

This is a fixup for merged code, not from the feature branch.

I think it's easy to ignore changing the next to release version. Please remember do it when merging to master. BTW, there is gen_lifecycle to check datamodel_lifecycle.ml, see here. I think similar methods can be used for datamodel_pool and datamodel_host too. It can be considered in the future.

BengangY · 2025-04-01T03:40:09Z

"...will be dropped sson."
A spelling mistake in the description of the second commit.

lindig · 2025-04-01T09:54:31Z

ocaml/xapi/xapi_pool.ml

+        Client.Host.get_console_idle_timeout ~rpc ~session_id
+          ~self:remote_coordinator
+      in
+      (* Configure SSH service parameters in local DB to setup local SSH


"to set up" ("setup" is a noun but you need a verb here)

lindig · 2025-04-01T09:57:40Z

ocaml/xapi/xapi_pool.ml

+         out yet, the joiner will start SSH service with timeout
+         host.ssh_enabled_timeout, which means SSH service in the joiner will
+         be disabled later than in the new coordinator. *)
+      let ssh_expiry = Db.Host.get_ssh_expiry ~__context ~self:host_ref in


The joining host is going through a reboot. What is the general policy for SSH over a reboot? If SSH was enabled before reboot, what is the state after reboot?

The joining host will not reboot, only xapi will have a restart to finish the joining.
While a host will go through a reboot when it is ejected from a pool. And I think in current design, SSH service status will be kept after reboot:

If SSH service is enabled without timeout or disabled, it will not change after reboot.

If SSH service is being enabled with a timeout, after reboot, it will be enabled with remaining timeout, and be disabled after time is up.

So the current policy is to sync the same status, including ssh_expiry, from the pool coordinator to the newly joined host. It relies on the reboot mechanism to ensure SSH is disabled eventually. Essentially, the newly joined host will follow the same process as the pool coordinator: reboot, check if the expiry time is greater than the current time, and trigger a disable.

So the current policy is to sync the same status, including ssh_expiry, from the pool coordinator to the newly joined host.

No, ssh_expiry will be now + ssh_enabled_timeout for the new joined host as we discussed offline, pls check the code.

Add "Changed" records for 2 APIs which were missed. Fix "param_release" for 3 added parameters. Signed-off-by: Gang Ji <gang.ji@cloud.com>

gangj · 2025-04-02T03:35:18Z

Force push to re-base to latest feature branch, which merged the latest master to fix the build failure.

changlei-li · 2025-04-02T07:52:46Z

ocaml/idl/datamodel_pool.ml

        ; param_name= "username"
        ; param_doc= "The username of the remote pool"
-        ; param_release= numbered_release "25.6.0-next"
+        ; param_release= numbered_release "25.7.0"


I think it's easy to ignore changing the next to release version. Please remember do it when merging to master. BTW, there is gen_lifecycle to check datamodel_lifecycle.ml, see here. I think similar methods can be used for datamodel_pool and datamodel_host too. It can be considered in the future.

ocaml/xapi/xapi_pool.ml

BengangY · 2025-04-02T08:31:30Z

ocaml/tests/common/test_common.ml

    ?(license_params = []) ?(edition = "free") ?(license_server = [])
    ?(local_cache_sr = Ref.null) ?(chipset_info = []) ?(ssl_legacy = false)
-    ?(last_software_update = Date.epoch) ?(last_update_hash = "") () =
+    ?(last_software_update = Date.epoch) ?(last_update_hash = "")


Why do we add these parameters to make_host? I don't see them been used.

They are used below: in line 181 and 182.

BengangY · 2025-04-02T08:53:02Z

ocaml/xapi/xapi_pool.ml

+      Db.Host.set_ssh_enabled ~__context ~self:host_ref ~value:ssh_enabled ;
+      Db.Host.set_ssh_enabled_timeout ~__context ~self:host_ref
+        ~value:ssh_enabled_timeout ;
+      Db.Host.set_console_idle_timeout ~__context ~self:host_ref


set_console_idle_timeout also updates /root/.bashrc. Should here call the API host.set_console_idle_timeout instead of updating DB directly?

Yes, it is clear with API call, changed to that.

BengangY · 2025-04-02T08:55:23Z

ocaml/xapi/xapi_pool.ml

+        Client.Host.get_ssh_enabled ~rpc ~session_id ~self:remote_coordinator
+      in
+      let ssh_enabled_timeout =
+        Client.Host.get_ssh_enabled_timeout ~rpc ~session_id


Will Client.Host.get_ssh_enabled_timeout send API call to the remote coordinator? If so, it will send 4 times API calls. Could it just send one time API call to fetch all these 4 parameters?

I think we can't do that without querying all the records. Or how?

Could Client.Host.get_record work?

That will query all the records of the host, I think it is expensive.

During pool join, create a new host obj in the remote pool coordinator DB with the same SSH settings as pool coordinator. Also configure SSH service locally before xapi restart which will persist after xapi restart. Signed-off-by: Gang Ji <gang.ji@cloud.com>

robhoes · 2025-04-02T13:38:27Z

ocaml/xapi/xapi_pool.ml

+      ( match ssh_enabled with
+      | true ->
+          Xapi_host.enable_ssh ~__context ~self:host_ref
+      | false ->
+          Xapi_host.disable_ssh ~__context ~self:host_ref
+      ) ;


This doesn't really belong here in this create_or_get_host_on_master function, where the purpose is to create a host record for joining host on the in pool's DB. It would be better to factor out these additional side effects to keep the logic clean(er).

From L958 actually.

BengangY reviewed Apr 1, 2025

View reviewed changes

gangj force-pushed the private/gangj/CP-53711 branch from bbfe5a4 to 01fe4e3 Compare April 1, 2025 04:36

lindig approved these changes Apr 1, 2025

View reviewed changes

Fixup

db38cb0

Add "Changed" records for 2 APIs which were missed. Fix "param_release" for 3 added parameters. Signed-off-by: Gang Ji <gang.ji@cloud.com>

gangj force-pushed the private/gangj/CP-53711 branch from 01fe4e3 to bd6c586 Compare April 2, 2025 03:33

gangj force-pushed the private/gangj/CP-53711 branch from bd6c586 to a09a2ed Compare April 2, 2025 03:43

changlei-li reviewed Apr 2, 2025

View reviewed changes

BengangY reviewed Apr 2, 2025

View reviewed changes

changlei-li approved these changes Apr 2, 2025

View reviewed changes

gangj force-pushed the private/gangj/CP-53711 branch from a09a2ed to 30bd072 Compare April 2, 2025 09:18

gangj force-pushed the private/gangj/CP-53711 branch from 30bd072 to a875364 Compare April 2, 2025 09:21

BengangY approved these changes Apr 2, 2025

View reviewed changes

gangj merged commit 6e6c0ed into xapi-project:feature/configure-ssh-phase2 Apr 2, 2025
17 checks passed

robhoes reviewed Apr 2, 2025

View reviewed changes

gangj mentioned this pull request Apr 7, 2025

CP-53711: Apply SSH settings in joiner before update_non_vm_metadata #6408

Merged

changlei-li mentioned this pull request Apr 7, 2025

CP-53802: Restore SSH service to default state in pool eject #6399

Merged

Conversation

gangj commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BengangY commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gangj Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gangj Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gangj commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gangj Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BengangY Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gangj Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

gangj commented Apr 1, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading

BengangY Apr 2, 2025 •

edited

Loading

gangj Apr 2, 2025 •

edited

Loading